Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Get rid of store adapter in aggregator #2143

Merged
merged 14 commits into from
Nov 27, 2024
Merged

Get rid of store adapter in aggregator #2143

merged 14 commits into from
Nov 27, 2024

Conversation

sfauvel
Copy link
Collaborator

@sfauvel sfauvel commented Nov 25, 2024 •
edited
Loading

Content

Replace the store using the legacy store adapter and replace them with a repository implementation in the Aggregator.

This PR includes:

  • Removing of StoreAdapter from persistence
  • Remove links to StoreAdapter for signer_registration and CertificatePending in Aggregator
  • Remove usage of AdapterError

Note

If pruning is unit tested, there is no test that verify that it's really plugged and worked in production.
It's just a configuration in build_upkeep_service function in the builder.rs file that activate it.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)

Comments

Issue(s)

Relates to #2118

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 25, 2024 •
edited
Loading

Test Results

    4 files  ± 0     51 suites  ±0   11m 50s ⏱️ ±0s
1 414 tests  - 45  1 414 ✅  - 45  0 💤 ±0  0 ❌ ±0 
1 625 runs   - 45  1 625 ✅  - 45  0 💤 ±0  0 ❌ ±0 

Results for commit d01beb7. ± Comparison against base commit 94495a4.

♻️ This comment has been updated with latest results.

@sfauvel sfauvel force-pushed the sfa/2118/get_rid_of_store_adapter_in_aggregator branch from 7363c3c to dae2e96 Compare November 26, 2024 09:41
@sfauvel sfauvel marked this pull request as ready for review November 26, 2024 10:23
@sfauvel sfauvel force-pushed the sfa/2118/get_rid_of_store_adapter_in_aggregator branch from dae2e96 to d0ba1ee Compare November 26, 2024 10:26
@sfauvel sfauvel force-pushed the sfa/2118/get_rid_of_store_adapter_in_aggregator branch 2 times, most recently from 20c0332 to e450847 Compare November 26, 2024 13:13
jpraynaud
jpraynaud reviewed Nov 26, 2024
View reviewed changes
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good 👍

I left few questions and suggestions.

mithril-aggregator/src/database/query/pending_certificate/get_pending_certificate.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/database/query/pending_certificate/get_pending_certificate.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/database/query/pending_certificate/save_pending_certificate.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/database/migration.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/database/record/certificate_pending.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/database/repository/signer_registration_store.rs Show resolved Hide resolved
mithril-aggregator/src/database/test_helper.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/database/test_helper.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/dependency_injection/builder.rs Show resolved Hide resolved
mithril-aggregator/src/services/epoch_service.rs Outdated Show resolved Hide resolved
dlachaume
dlachaume approved these changes Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

mithril-aggregator/src/database/repository/pending_certificate_repository.rs Outdated Show resolved Hide resolved
mithril-aggregator/src/dependency_injection/builder.rs Outdated Show resolved Hide resolved
@sfauvel sfauvel force-pushed the sfa/2118/get_rid_of_store_adapter_in_aggregator branch from d78085f to 61fd2ee Compare November 26, 2024 17:53
@sfauvel sfauvel force-pushed the sfa/2118/get_rid_of_store_adapter_in_aggregator branch from 28035c4 to 6bad9ab Compare November 27, 2024 15:30
@sfauvel
Rename certificatecolumn and record field to pending_certificate
278ef1d
@sfauvel
chore: upgrade crate versions
d01beb7 
* mithril-persistence from `0.2.36` to `0.2.37`
* mithril-aggregator from `0.5.114` to `0.5.115`
jpraynaud
jpraynaud approved these changes Nov 27, 2024
View reviewed changes
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@sfauvel sfauvel merged commit e38a254 into main Nov 27, 2024
50 checks passed
@sfauvel sfauvel deleted the sfa/2118/get_rid_of_store_adapter_in_aggregator branch November 27, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlachaume dlachaume dlachaume approved these changes

@jpraynaud jpraynaud jpraynaud approved these changes

@Alenar Alenar Alenar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sfauvel @Alenar @jpraynaud @dlachaume